Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

refactor: avoid unnecessary calls to platforms.DefaultSpec() #5756

Merged
merged 1 commit into from
Feb 19, 2025
Merged

refactor: avoid unnecessary calls to platforms.DefaultSpec() #5756

merged 1 commit into from
Feb 19, 2025
+18 −6

Conversation

profnandaa
Copy link
Collaborator

I came across a number of calls to platforms.DefaultSpec() that were not necessary. That prompted me to take a look various other similar places.

At least on Windows, the call is a has some cost till doing a sycall RtlGetNtVersionNumbers().

@profnandaa
refactor: avoid unnecessary calls to platforms.DefaultSpec()
cc0cb08 
I came across a number of calls to `platforms.DefaultSpec()`
that were not necessary. That prompted me to take a look
various other similar places.

At least on Windows, the call is a has some cost till
doing a sycall `RtlGetNtVersionNumbers()`.

Signed-off-by: Anthony Nandaa <[email protected]>
@profnandaa profnandaa force-pushed the refactor_defaultplatform branch from bd98381 to cc0cb08 Compare February 19, 2025 04:37
@profnandaa profnandaa marked this pull request as ready for review February 19, 2025 05:23
iankingori
iankingori approved these changes Feb 19, 2025
View reviewed changes
Copy link
Contributor

@iankingori iankingori left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes sense, looks like DefaultSpec was used as a type placeholder then overwritten entirely

@crazy-max
Copy link
Member

At least on Windows, the call is a has some cost till doing a sycall RtlGetNtVersionNumbers().

I think this should be fixed upstream instead: https://github.com/containerd/platforms/blob/e3566b8ff1994b8dc88bae5768d32830e0cd0cfd/defaults_windows.go#L29 so windows.RtlGetNtVersionNumbers() is cached. WDYT @cpuguy83 @thaJeztah?

@thaJeztah
Copy link
Member

thaJeztah commented Feb 19, 2025 •
edited
Loading

Hm.. right; I wasn't aware this call didn't come without cost.

Assuming that versions doesn't change during the runtime of the (buildkit) daemon, perhaps the platforms module should use a sync.Once or sync.OnceValue for the version (or default spec as a whole)

@thaJeztah
Copy link
Member

Also curious about the Normalize call; I suspect DefaultSpec() would always be normalized, so normalising should likely only be needed if a custom platform is set (not the default).

@cpuguy83
Copy link
Member

Seems reasonable to cache it.
Also something that could probably be wrapped internally in buildkit.

@tonistiigi tonistiigi merged commit e9fa57a into moby:master Feb 19, 2025
105 checks passed
@profnandaa profnandaa deleted the refactor_defaultplatform branch February 20, 2025 06:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tonistiigi tonistiigi tonistiigi approved these changes

@iankingori iankingori iankingori approved these changes

Assignees
No one assigned
Labels
area/dockerfile area/exporter area/frontend area/solver
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@profnandaa @crazy-max @thaJeztah @cpuguy83 @tonistiigi @iankingori